-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat/CK-147] 골룸 생성 페이지 고도화 및 유효성 검사를 구현한다 #114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
커밋 기록을 다 보며 확인했는데 계속 훅이 진화하는게 인상적이었어요! ㅋㅋㅋㅋㅋ
너무 고생하셨고, 리뷰 내용만 확인 부탁드려요! vㅔ리 굳입니다!!
- 영상을 보았을 때 하나의 input validation 만 실패하더라도 form 에 있는 모든 Input 의 validation 이 실패하는 것처럼 보이는 것 같은데 맞나용?
import { isCurrentOrFutureDate } from '@utils/_common/isCurrentOrFutureDate'; | ||
|
||
it('과거의 날짜를 입력하면 false를 반환한다', () => { | ||
const isValidateDate = isCurrentOrFutureDate('2022-03-14'); | ||
|
||
expect(isValidateDate).toBe(false); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
순수하게 궁금한게 이 하나의 테스트로 테스트 커버리지가 80% 이상이 나왔었나용?? 😮
const [formState, setFormState] = useState<T>(initialState); | ||
const [error, setError] = useState<FormErrorType>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초기 값을 넣어주지 않아 undefined
가 나올경우가 있을 것 같네요..! null
값이나 {}
빈 객체를 초기 값으로 할당하는 것은 어떨까요? 🫡
@@ -45,7 +129,6 @@ const useFormInput = <T extends object>(initialState: T) => { | |||
}, | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로대쉬가 그리워지는 로직이군요..! 😀
return (currentValue as any)[part]; | ||
}, formState); | ||
|
||
const isValid = validate(String(fieldValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validateInputValue
와 handleSubmit
함수에서 유효성 검사 로직이 중복되는 것처럼 보이는데 의도된 상황일까여?? 🙂
@@ -62,6 +145,8 @@ const useFormInput = <T extends object>(initialState: T) => { | |||
formState, | |||
handleInputChange, | |||
resetFormState, | |||
error, | |||
handleSubmit, | |||
}; | |||
}; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배열 핸들링이 코드에 들어가 살짝 복잡해지는 감이 있는데, 아래와 같이 분리해보는건 어떨까여?? 😁
const handleArrayUpdate = (baseName: string, arrayIndex: string, arrayPropName: string) => {
// 배열 업데이트 로직
};
}; | ||
|
||
const useFormInput = <T extends object>( | ||
initialState: T, | ||
validations?: ValidationsType | ||
) => { | ||
const [formState, setFormState] = useState<T>(initialState); | ||
const [error, setError] = useState<FormErrorType>(); | ||
const [error, setError] = useState<FormErrorType>({}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 아까 리뷰 남겼던 부분이 여기서 이미 해결이 되었군요..!!! 👍👍👍👍
}; | ||
}); | ||
} | ||
if (isFormValid()) callback(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callback
보다는 callbackFn
혹은 callbackFunction
같이 조금 더 명시적이면 인자로 받은 콜백 함수겠구나~ 하고 판단하기 조금 더 편하지 않을까 생각이 드네요! 어떻게 생각하시나용
@@ -0,0 +1 @@ | |||
export const isEmptyString = (value: string) => value.length === 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 개인적으로 유틸 함수의 경우에는 팀원 모두가 자주 쓰게 될 것 같아서 모두 간단한 설명과 사용예시를 담은 주석을 추가하는건 어떨까용?? 😀
@@ -0,0 +1 @@ | |||
export const isNumeric = (value: string) => /^[1-9]+\d*$/.test(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석 부탁려용~~
@@ -0,0 +1,3 @@ | |||
export const isValidMaxLength = (value: string, max: number) => { | |||
return value.length <= max; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주우썩 부탁드려용~~
<textarea | ||
id={props.name} | ||
name={props.name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id는 왜 지정해준건지 궁금해요!!
style?: { [key: string]: string }; | ||
}; | ||
|
||
const InputField = ({ ...props }: InputFieldProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러 props를 받아서 Input을 사용할 수 있게 한거 너무 좋은 것 같아요!! 우디 고민한 티가 많이나고 너무 수고많았어요ㅠㅠ 조금 더 다듬어서 서비스 내 모든 input에 적용해켜봅시당!!
지금 당장은 아니고 리팩토링할 때 했으면 하는 부분들 여기에 정리해놨습니다!
https://www.notion.so/d0b3ea6e9cf44703ab9c3a10bb6e1b96
|
||
const useFormInput = <T extends object>(initialState: T) => { | ||
export type FormErrorType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 선언부 모두 myTypes 폴더로 분리 하는게 좋을 것 같아요~!
const [error, setError] = useState<FormErrorType>({}); | ||
|
||
const validateInputValue = (name: string, inputValue: string) => { | ||
if (typeof validations?.[name] !== 'function') return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 선언부를 보면 validations는 함수를 string인 key와 function인 value를 갖는 객체인 것 같은데, value의 타입이 'function'이 아닌 경우는 왜 구분해준건지 궁금해요!
export type ValidationReturnType = { | ||
ok: boolean; | ||
message?: string; | ||
updateOnFail?: boolean; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 타입도 조건부로 선언해야할 것 같아요! ok가 true일 때는 message는 있으면 안되고, ok가 false일 때만 message가 ValidationFunctionType의 반환값에 들어있어야 하는게 아닌가요?!
Object.keys(validations).forEach((key) => { | ||
const result = validations[key](String(getNestedValue(formState, key))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체의 정확한 타입 추론을 위해서 util 폴더의 invarientType에 있는 유틸함수르 이용해서 object.keys()를 사용해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우디 너무너무 수고햐셨어요~~!!!!! 코멘트 확인해주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vㅔ리 굳이에용~~
📌 작업 이슈 번호
CK-147
✨ 작업 내용
💬 리뷰어에게 남길 멘트
혹여나 잘 이해가 가지 않는 부분이 있다면 언제든 말씀해주세요
화면공유하고 따로 설명드리겠습니다😭
잘 부탁드립니다!
2023-08-14.4.05.00.mov